bugfix (routing): fix incorrect min_vehicles#1405
Open
np96 wants to merge 2 commits into
Open
Conversation
📝 WalkthroughWalkthroughSolver selection now treats fleet_size >= min_vehicles as the fixed-route path; OX recombiner skips variable route-count search in fixed-route mode and rejects offspring on route-count mismatch; cycle finder adds buffer bounds checks; move-candidates uses async zeroing; tests add a parametrized regression ensuring min_vehicles is respected. ChangesIssue
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
np96
force-pushed
the
bug/min_vehicles_fix
branch
2 times, most recently
from
d8f4ad9 to
f51933b
Compare
np96
force-pushed
the
bug/min_vehicles_fix
branch
from
f51933b to
de69f9e
Compare
np96
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
python/cuopt/cuopt/tests/routing/test_solver.py (1)
335-335:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse the established solution message accessor in the assert payload.
This file consistently uses
get_message(), but this assertion usesget_error_message(). If that accessor is unavailable, a failing status check can raiseAttributeErrorand mask the real solver failure reason.Suggested fix
- assert sol.get_status() == 0, sol.get_error_message() + assert sol.get_status() == 0, sol.get_message()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/cuopt/cuopt/tests/routing/test_solver.py` at line 335, Replace the assert payload that references the non-standard accessor get_error_message() with the established accessor get_message(); specifically, update the assertion that checks sol.get_status() to use sol.get_message() as the second argument (referencing sol.get_status(), sol.get_message(), and removing get_error_message()) so the failure message uses the standard solution message accessor.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@python/cuopt/cuopt/tests/routing/test_solver.py`:
- Line 335: Replace the assert payload that references the non-standard accessor
get_error_message() with the established accessor get_message(); specifically,
update the assertion that checks sol.get_status() to use sol.get_message() as
the second argument (referencing sol.get_status(), sol.get_message(), and
removing get_error_message()) so the failure message uses the standard solution
message accessor.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d55296de-e57d-4d47-85a8-6696511bc14a
📒 Files selected for processing (5)
cpp/src/routing/crossovers/ox_recombiner.cuhcpp/src/routing/local_search/cycle_finder/cycle.hppcpp/src/routing/local_search/move_candidates/move_candidates.cuhcpp/src/routing/solver.cupython/cuopt/cuopt/tests/routing/test_solver.py
✅ Files skipped from review due to trivial changes (1)
- cpp/src/routing/local_search/move_candidates/move_candidates.cuh
🚧 Files skipped from review as they are similar to previous changes (3)
- cpp/src/routing/crossovers/ox_recombiner.cuh
- cpp/src/routing/local_search/cycle_finder/cycle.hpp
- cpp/src/routing/solver.cu
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fix min_vehicles ignored with vehicle fixed costs (#904)
When vehicle_fixed_costs were set and min_vehicles == fleet_size, the
solver returned fewer vehicles than min_vehicles and, in debug builds,
crashed with device-side asserts. Three coordinated issues:
solver.cu: engage the fixed-route loop whenever the fleet can satisfy
min_vehicles (fleet_size >= min_vehicles), not only on exact equality,
so target_vehicles is honored in more cases.
move_candidates.cuh: move_path::reset() zeroed n_insertions via
device_scalar::set_value_async(), which copies from a host stack local.
reset() runs inside a captured CUDA graph, so that host source dangled
by graph-launch time and left n_insertions garbage (~32k) — perform_moves
then launched tens of thousands of out-of-bounds blocks. Use the
memset-based set_value_to_zero_async(), which is capture-safe.
ox_recombiner.cuh: in fixed_route mode (fleet_size == min_vehicles), the
VEHICLE_FIXED_COST branch enabled optimal-routes search, letting
Bellman-Ford vary the route count. That contradicts the fixed vehicle
count: it broke the recreate_solution invariant (routes removed == routes
added) and drifted the solution below min_vehicles. Skip optimal-routes
search when the count is fixed, and reject any count-mismatched offspring
instead of applying it.
Issue
#904
Checklist